[PAR-848] Configurable Express Button Styles - #69

🐛 correctness

A save-express-checkout-settings webhook payload that omits buttonStyle silently wipes the merchant's stored style instead of leaving it untouched.

$payload['buttonStyle'] ?? null cannot distinguish "key absent" from "explicitly cleared", so transformToDomainModel() builds ExpressCheckoutSettings($configs, null) and setExpressCheckoutSettings() upserts it.

A merchant configures a style, then any client that posts only {topic, expressCheckoutConfigs} — an older portal build, a targeted page-enable/disable call, a retried partial payload — resets buttonStyle to null and the storefront button reverts to defaults with no error.

The PR's own test at ConfigurationWebhookAPITest.php:2303 (assertNull($persisted->getButtonStyle()) after a payload with no buttonStyle) locks this behaviour in.

Not changing this, but flagging it as a product decision rather than a bug.

save-express-checkout-settings is a full-replace save: expressCheckoutConfigs behaves exactly the same way, so a payload that omits it clears the configs too. Under those semantics buttonStyle going back to null when it is absent is correct, and the test at ConfigurationWebhookAPITest.php:2303 documents that intent rather than locking a defect.

If the topic is meant to be a partial patch instead, then this is a real bug and the fix is distinguishing "key absent" from "explicitly cleared" — but that is a contract question for the portal (sequra/merchant-portal-frontend#1731), not something to guess at here. Leaving the thread open for that.

🔒 security

A merchant-controlled string is echoed verbatim into the public storefront availability response with no escaping, no content validation and no length bound, making every deployed plugin solely responsible for preventing stored XSS.

ConfigurationWebhookAPITest.php:2348 explicitly asserts that '{"attributeThisReleaseNeverHeardOf":"<script>x</script>", ...}' survives the round trip untouched. That string is persisted and later returned by isAvailable()/isAvailableForGuest().

Any integration that interpolates buttonStyle into an inline <style>/<script>/attribute rather than passing it through JSON.parse + the CDN allow-list gets stored XSS on every product and cart page.

The core has the choke point here (one toArray()) and does nothing with it. At minimum re-encode via json_encode(json_decode(...)) to strip anything that isn't valid JSON structure, and cap the length.

Partly taken, partly declined — leaving open for your call.

Taken: the length bound is in (62d572a), and the style must now decode to a JSON object, so the blob is no longer unbounded or structurally arbitrary.

Declined: the json_encode(json_decode(...)) re-encode. It does not sanitise values — PHP does not escape < by default, and with JSON_HEX_TAG it becomes \u003C, which JSON.parse turns straight back into <. So it only helps a plugin that interpolates the blob into an inline <script>, and that plugin has larger problems than this field.

On the escaping premise: the validation exists, in integration-assets#669, and it is a whitelist at the point of use — five known keys, a hex regex for colours, a token map for the radius, a clamped font size, all applied through style.setProperty and never through the markup template. The attributeThisReleaseNeverHeardOf value in that test is not one of the five keys, so it is dropped before its value is ever read; put <script> in backgroundColor and it fails the regex. The blob reaching the plugin as a data- attribute still needs ordinary attribute escaping, which esc_attr / escapeHtmlAttr already do for every attribute.

The core deliberately does not know the key names: they are owned by the portal and the button page so that adding one does not need a release of this library and an update of every installed plugin. Checking the shape (object, bounded) is schema-agnostic and compatible with that; checking the contents would not be.

🐛 correctness

Replacing the entity's explicit persistence mapping with $this->expressCheckoutSettings->toArray() couples the on-disk storage format to the domain model's serialization, so any future field added to the domain silently changes what is written to storage.

The deleted code spelled out exactly which keys are persisted.

Now, if someone adds a derived / presentation field to ExpressCheckoutSettings::toArray() (a resolved default style, a computed enabledPages list), it is written into every stored row without anyone touching the DataAccess layer — and inflate(), which reads only expressCheckoutConfigs and buttonStyle, silently drops it on the next load, producing an asymmetric round trip.

Keep the entity's mapping explicit, or add a matching ExpressCheckoutSettings::fromArray() so the two halves stay symmetric by construction.

Declined. The asymmetry is real, but there is no defect today, and the explicit mapping was removed on purpose in 2fbc9c1 to stop the same field being spelled out in three places. Reinstating it, or adding a fromArray() to pair with it, is more code for a hypothetical field. Noted as a footgun rather than fixed.

♻️ simplification

GuestExpressCheckoutAvailabilityResponse now duplicates a third field ($buttonStyle) already declared identically in ExpressCheckoutAvailabilityResponse, along with its docblock, constructor assignment and toArray() entry.

The two classes now hold identical protected $available + protected $buttonStyle declarations, identical @var string|null docblocks, identical assignments and identical 'buttonStyle' => $this->buttonStyle lines — the guest response is ExpressCheckoutAvailabilityResponse plus one array field.

A fourth style-related field means editing both files again, and a fix applied to only one (e.g. re-encoding the blob, per the escaping finding) silently leaves the guest endpoint unprotected.

Have the guest response extend ExpressCheckoutAvailabilityResponse and add only availableCountries.

Declined. The argument that carried this one was that a fix applied to one response would leave the other unprotected — and that dissolves now that the validation lives in the domain model rather than in either DTO. What is left is cosmetic duplication of two fields.

♻️ simplification

GetExpressCheckoutSettingsResponse::toArray() hand-rebuilds the settings shape field by field even though the same PR just refactored the ORM entity to delegate to ExpressCheckoutSettings::toArray().

Adding buttonStyle required editing this file (two new lines plus a second null-guard ternary) and the domain toArray() and the entity — three places for one field.

This could be $this->expressCheckoutSettings ? $this->expressCheckoutSettings->toArray() : ['expressCheckoutConfigs' => [], 'buttonStyle' => null] merged with availablePages.

As written, the persisted shape and the webhook GET shape are two independent definitions kept in sync by hand; the next field will drift between them.

Declined, same reasoning as the guest-response thread: cosmetic duplication with no correctness consequence now that the shape is validated once in the domain. Worth doing the next time this response changes for another reason.